[CL] Changes from audit results(1/2) #5568

mattverse · 2023-06-19T10:36:55Z

Closes: #XXX

What is the purpose of the change

These are changes from the audit results.

Testing and Verifying

Ensured that existing test cases pass

Documentation and Release Note

Does this pull request introduce a new feature or user-facing behavior changes?
Changelog entry added to Unreleased section of CHANGELOG.md?

Where is the change documented?

Specification (x/{module}/README.md)
Osmosis documentation site
Code comments?
N/A

mattverse · 2023-06-19T10:37:48Z

osmoutils/accum/accum.go

@@ -294,7 +294,7 @@ func (accum *AccumulatorObject) UpdatePosition(name string, numShares sdk.Dec) e
 // All intervalAccumulationPerShare DecCoin value must be non-negative. They must also be a superset of the
 // old accumulator value associated with the position.
 func (accum *AccumulatorObject) UpdatePositionIntervalAccumulation(name string, numShares sdk.Dec, intervalAccumulationPerShare sdk.DecCoins) error {
-	if numShares.Equal(sdk.ZeroDec()) {
+	if numShares.IsZero() {


Using IsZero directly avoids creating and using a new sdk.Dec object

mattverse · 2023-06-19T10:38:24Z

osmoutils/accum/accum_test.go

@@ -1163,7 +1163,7 @@ func (suite *AccumTestSuite) TestGetPositionSize() {
 			// Get position size from valid address (or from nonexistant if address does not exist)
 			positionSize, err := curAccum.GetPositionSize(positionName)

-			if tc.changedShares.GT(sdk.ZeroDec()) {
+			if tc.changedShares.IsPositive() {


ditto for GT(sdk.ZeroDec) or LT(sdk.ZeroDec), using IsPositive or IsNegative methods avoid creating unnecessasry sdk.Dec object

mattverse · 2023-06-19T10:40:04Z

x/concentrated-liquidity/incentives.go


-		// If there is no share to be incentivized for the current uptime accumulator, we leave it unchanged
-		if qualifyingLiquidity.LT(sdk.OneDec()) {


This change mainly is from here, right now, we are entering loop checking if qualifyingLiquidity.LT(sdk.OneDec()) and then continue checking this every iteration.
This can be refactored into simply having this check before the for iteration

mattverse · 2023-06-19T10:40:49Z

x/concentrated-liquidity/incentives.go

-	err = k.ensurePositionOwner(ctx, sender, position.PoolId, positionId)
-	if err != nil {
-		return sdk.Coins{}, sdk.Coins{}, err
+	if sender.String() != position.Address {


Directly checking this instead of using ensurePositionOwner allows us to save unnecessary iteration over all positions in the pool

mattverse · 2023-06-19T10:43:52Z

x/concentrated-liquidity/lp.go

@@ -321,21 +321,19 @@ func (k Keeper) addToPosition(ctx sdk.Context, owner sdk.AccAddress, positionId
 		return 0, sdk.Int{}, sdk.Int{}, types.PositionSuperfluidStakedError{PositionId: position.PositionId}
 	}

-	// Withdraw full position.
-	amount0Withdrawn, amount1Withdrawn, err := k.WithdrawPosition(ctx, owner, positionId, position.Liquidity)
+	positions, err := k.GetAllPositionIdsForPoolId(ctx, types.PositionPrefix, position.PoolId)


Doing this check and checking if this would be the last position before the actual WithdrawPosition rather than doing this after calling WithdrawPosition allows us to save computation

Wait isn't this linear in the number of position ID's for a pool? Thats way too expensive to run on addToPosition?

Yeah actually removing my approve, I think we have to revert this lp.go change before its safe to merge

Good to merge after this is reverted

mattverse · 2023-06-19T10:44:11Z

x/concentrated-liquidity/lp_test.go

-				err = concentratedLiquidityKeeper.EnsurePositionOwner(s.Ctx, owner, config.poolId, config.positionId)
-				s.Require().Error(err, types.NotPositionOwnerError{PositionId: config.positionId, Address: owner.String()})


ensurePositionOwner method is now deleted, nor do we need this check in this test case

mattverse · 2023-06-19T10:44:35Z

x/concentrated-liquidity/position.go

-func (k Keeper) ensurePositionOwner(ctx sdk.Context, sender sdk.AccAddress, poolId uint64, positionId uint64) error {
-	isOwner := k.isPositionOwner(ctx, sender, poolId, positionId)
-	if !isOwner {
-		return types.NotPositionOwnerError{
-			PositionId: positionId,
-			Address:    sender.String(),
-		}
-	}
-	return nil
-}
-
-// isPositionOwner returns true if the given positionId is owned by the given sender inside the given pool.
-func (k Keeper) isPositionOwner(ctx sdk.Context, sender sdk.AccAddress, poolId uint64, positionId uint64) bool {
-	return ctx.KVStore(k.storeKey).Has(types.KeyAddressPoolIdPositionId(sender, poolId, positionId))
-}
-


not used any more, thus deletion

mattverse · 2023-06-19T10:45:09Z

x/concentrated-liquidity/position_test.go

@@ -410,103 +410,6 @@ type positionOwnershipTest struct {
 	poolId         uint64
 }

-func (s *KeeperTestSuite) runIsPositionOwnerTest(test positionOwnershipTest) {


The method that was being tested is now deleted thus test is removed as well

mattverse · 2023-06-19T10:46:12Z

x/incentives/keeper/distribute.go

+	// defense in depth
+	// this should never happen in practice since gauge passed in should always be an active gauge.
+	if remainEpochs == uint64(0) {
+		return nil, fmt.Errorf("gauge with id of %d is not active", gauge.Id)
+	}
+


this should never happen in practice, but if it does, this can lead to chain halt since this would lead to division by zero(=panic) in Begin block, which can potentially lead to chain halt. Thus adding defense in depth

x/concentrated-liquidity/math/math.go

ValarDragon · 2023-06-19T12:39:06Z

x/concentrated-liquidity/swaps.go

-		swapState.amountSpecifiedRemaining = swapState.amountSpecifiedRemaining.SubMut(amountOut)
-		swapState.amountCalculated = swapState.amountCalculated.AddMut(amountIn.Add(spreadRewardChargeTotal))


fwiw, I personally find it easier to see whats going on with the equal signs

let's bring back the equal signs

ValarDragon · 2023-06-19T12:41:40Z

x/gamm/pool-models/internal/cfmm_common/lp.go

@@ -25,7 +25,7 @@ func CalcExitPool(ctx sdk.Context, pool types.CFMMPoolI, exitingShares sdk.Int,
 	var refundedShares sdk.Dec
 	if !exitFee.IsZero() {
 		// exitingShares * (1 - exit fee)
-		oneSubExitFee := sdk.OneDec().SubMut(exitFee)
+		oneSubExitFee := sdk.OneDec().Sub(exitFee)


not actually a bug (as oneDec is a new copy), but LGTM.

Anyways not on a hot path

ValarDragon

All changes LGTM

Though not sure I agree with some of the removing = for already mutative ops.

The changing things to IsZero / IsPositive is great!

p0mvn · 2023-06-19T15:38:28Z

x/incentives/keeper/distribute.go

+	// defense in depth
+	// this should never happen in practice since gauge passed in should always be an active gauge.
+	if remainEpochs == uint64(0) {
+		return nil, fmt.Errorf("gauge with id of %d is not active", gauge.Id)
+	}


Should we also be adding a check in validate basic that NumEpochsPaidOver can't be 0?

@p0mvn which validate basic are you referring to ?

p0mvn

LGTM

Would like to bump up Dev's concern about getting all positions in AddToPosition prior to merge

ValarDragon · 2023-06-19T15:49:55Z

x/concentrated-liquidity/lp.go

@@ -321,21 +321,19 @@ func (k Keeper) addToPosition(ctx sdk.Context, owner sdk.AccAddress, positionId
 		return 0, sdk.Int{}, sdk.Int{}, types.PositionSuperfluidStakedError{PositionId: position.PositionId}
 	}

-	// Withdraw full position.
-	amount0Withdrawn, amount1Withdrawn, err := k.WithdrawPosition(ctx, owner, positionId, position.Liquidity)
+	positions, err := k.GetAllPositionIdsForPoolId(ctx, types.PositionPrefix, position.PoolId)


Yeah actually removing my approve, I think we have to revert this lp.go change before its safe to merge

Good to merge after this is reverted

Co-authored-by: Dev Ojha <[email protected]>

* repro panic trigger and fix bound check * fix comments

) Closes: #XXX ## What is the purpose of the change Related to: #5550 Documenting latest decisions around tick and price conversions ## Testing and Verifying This change is a trivial rework / code cleanup without any test coverage. ## Documentation and Release Note - [ ] Does this pull request introduce a new feature or user-facing behavior changes? - [ ] Changelog entry added to `Unreleased` section of `CHANGELOG.md`? Where is the change documented? - [ ] Specification (`x/{module}/README.md`) - [ ] Osmosis documentation site - [ ] Code comments? - [ ] N/A

Closes: #XXX ## What is the purpose of the change The following PR introduces a functional test that: - Creates every type of balancer position that can be created - Bonded superfluid - Unbonded superfluid (locked) - Unbonded superfluid (unlocking) - Vanilla lock (locked) - Vanilla lock (unlocking) - No lock - It then migrates each position, asserting invariants after each position is migrated - Finally, an overall invariant is run after all positions have been migrated, asserting that all funds are accounted for in some way The next PR subsequent to this will be adding randomization to the inputs in order to make the test non deterministic. ## Testing and Verifying Functional test above added ## Documentation and Release Note - [ ] Does this pull request introduce a new feature or user-facing behavior changes? - [ ] Changelog entry added to `Unreleased` section of `CHANGELOG.md`? Where is the change documented? - [ ] Specification (`x/{module}/README.md`) - [ ] Osmosis documentation site - [ ] Code comments? - [ ] N/A

@ValarDragon

… <> sqrt price (#5541) Closes: #5516 > **Note to reviewer:** the only real state machine change is in `tick.go` and heavily mirrors @ValarDragon's PR here: #5522 > The rest of the changes are related to function renames/test refactors. ## What is the purpose of the change This PR expands on #5522 and updates all price to tick conversions to use SqrtPriceToTick instead. ## Testing and Verifying The new function is tested in `math/tick_test.go` The original attack vector is also converted into an edge case test in `position_test.go` ## Documentation and Release Note - [ ] Does this pull request introduce a new feature or user-facing behavior changes? - [ ] Changelog entry added to `Unreleased` section of `CHANGELOG.md`? Where is the change documented? - [ ] Specification (`x/{module}/README.md`) - [ ] Osmosis documentation site - [ ] Code comments? - [ ] N/A

mattverse · 2023-06-20T06:30:46Z

lp.go has been reverted to original state, planning to merge this upon seeing CI pass

mattverse · 2023-06-20T06:52:07Z

Oh I can't merge it unless a third reviewer lifts the existing requested change review

Some optimizations

e6f552d

mattverse requested review from stackman27, nicolaslara, p0mvn and czarcas7ic as code owners June 19, 2023 10:36

mattverse added V:state/compatible/no_backport State machine compatible PR, depends on prior breaks A:no-changelog labels Jun 19, 2023

github-actions bot added C:simulator Edits simulator or simulations C:x/concentrated-liquidity C:x/gamm Changes, features and bugs related to the gamm module. C:x/incentives C:x/mint C:x/superfluid C:x/twap Changes to the twap module labels Jun 19, 2023

mattverse commented Jun 19, 2023

View reviewed changes

ValarDragon reviewed Jun 19, 2023

View reviewed changes

x/concentrated-liquidity/math/math.go Outdated Show resolved Hide resolved

ValarDragon reviewed Jun 19, 2023

View reviewed changes

ValarDragon approved these changes Jun 19, 2023

View reviewed changes

p0mvn reviewed Jun 19, 2023

View reviewed changes

p0mvn approved these changes Jun 19, 2023

View reviewed changes

ValarDragon requested changes Jun 19, 2023

View reviewed changes

mattverse and others added 9 commits June 20, 2023 14:45

Update x/concentrated-liquidity/math/math.go

c223e07

Co-authored-by: Dev Ojha <[email protected]>

[CL]: Fix incorrect bound check/chain halt vector (#5557)

440bea8

* repro panic trigger and fix bound check * fix comments

Make go tests only run if relevant (#5569)

3400a02

Merge branch 'main' into mattverse/informal

7044f88

Still fixing merge conlfict

5d0c6e8

Revert lp.go change as commented

3775150

ValarDragon approved these changes Jun 20, 2023

View reviewed changes

ValarDragon merged commit 0cea7aa into main Jun 20, 2023

ValarDragon deleted the mattverse/informal branch June 20, 2023 08:40

mtsitrin mentioned this pull request Dec 6, 2023

upstream recent fixes dymensionxyz/osmosis#34

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CL] Changes from audit results(1/2) #5568

[CL] Changes from audit results(1/2) #5568

mattverse commented Jun 19, 2023

mattverse Jun 19, 2023

mattverse Jun 19, 2023

mattverse Jun 19, 2023

ValarDragon Jun 19, 2023

mattverse Jun 19, 2023

mattverse Jun 19, 2023

ValarDragon Jun 19, 2023

ValarDragon Jun 19, 2023 •

edited

Loading

mattverse Jun 19, 2023

mattverse Jun 19, 2023 •

edited

Loading

mattverse Jun 19, 2023

mattverse Jun 19, 2023

ValarDragon Jun 19, 2023

mattverse Jun 20, 2023

ValarDragon Jun 19, 2023

ValarDragon left a comment

p0mvn Jun 19, 2023

mattverse Jun 20, 2023

p0mvn left a comment

ValarDragon Jun 19, 2023 •

edited

Loading

mattverse commented Jun 20, 2023

mattverse commented Jun 20, 2023


		// If there is no share to be incentivized for the current uptime accumulator, we leave it unchanged
		if qualifyingLiquidity.LT(sdk.OneDec()) {

		err = concentratedLiquidityKeeper.EnsurePositionOwner(s.Ctx, owner, config.poolId, config.positionId)
		s.Require().Error(err, types.NotPositionOwnerError{PositionId: config.positionId, Address: owner.String()})

		swapState.amountSpecifiedRemaining = swapState.amountSpecifiedRemaining.SubMut(amountOut)
		swapState.amountCalculated = swapState.amountCalculated.AddMut(amountIn.Add(spreadRewardChargeTotal))

[CL] Changes from audit results(1/2) #5568

[CL] Changes from audit results(1/2) #5568

Conversation

mattverse commented Jun 19, 2023

What is the purpose of the change

Testing and Verifying

Documentation and Release Note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon Jun 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattverse Jun 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0mvn left a comment

Choose a reason for hiding this comment

ValarDragon Jun 19, 2023 • edited Loading

Choose a reason for hiding this comment

mattverse commented Jun 20, 2023

mattverse commented Jun 20, 2023

ValarDragon Jun 19, 2023 •

edited

Loading

mattverse Jun 19, 2023 •

edited

Loading

ValarDragon Jun 19, 2023 •

edited

Loading